Skip to content

Adding dict support for renaming columns#671

Open
henrydingliu wants to merge 1 commit intocasact:masterfrom
henrydingliu:master
Open

Adding dict support for renaming columns#671
henrydingliu wants to merge 1 commit intocasact:masterfrom
henrydingliu:master

Conversation

@henrydingliu
Copy link
Collaborator

Addressing #660

@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.04%. Comparing base (67f95de) to head (baccf52).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
chainladder/core/pandas.py 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
+ Coverage   85.03%   85.04%   +0.01%     
==========================================
  Files          85       85              
  Lines        4890     4896       +6     
  Branches      627      629       +2     
==========================================
+ Hits         4158     4164       +6     
  Misses        522      522              
  Partials      210      210              
Flag Coverage Δ
unittests 85.04% <87.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrydingliu henrydingliu requested a review from genedan January 30, 2026 18:25
@henrydingliu
Copy link
Collaborator Author

@genedan bumping this to the top of your inbox

@genedan
Copy link
Collaborator

genedan commented Feb 8, 2026

Got it. I'll take a look at this tomorrow.



def test_rename_exception(genins, clrd) -> None:
# Test incorrect value argument - misspelling of string.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understood this comment. To me it seems to be testing 2 things at once, trying to map a new name to a non-existing column, and attempting to do a rename meant for columns, but accidentally applied to the origin instead, and then alerting the user by raising an error.

However, doing something similar in pandas doesn't actually cause an error. For example, if we create a DataFrame with columns A and B but pass a mapping {'C': 'bar'} whose key not only does not match either column but also gets accidentally passed to the index parameter, we just get a copy of the original DataFrame with no error.

import pandas as pd
df = pd.DataFrame({"A": [1, 2, 3], "B": [4, 5, 6]})
df
Out[4]: 
   A  B
0  1  4
1  2  5
2  3  6
df.rename(index={'C': 'bar'})
Out[5]: 
   A  B
0  1  4
1  2  5
2  3  6

I believe this was intentional by design for pandas, for example if we want to apply a mapping where a DataFrame only hit a subset of the keys (or even none), we'd still want it to work.

Eventually, to match the functionality of DataFrame.rename, we'll need to remove the value parameter and supply the mapping to other parameters, working our way towards matching the signature of DataFrame.rename:

DataFrame.rename(mapper=None, *, index=None, columns=None, axis=None, copy=<no_default>, inplace=False, level=None, errors='ignore')

But I remember we talked about moving these further changes to other issues. This PR still makes sense given the chainladder's current behavior, so I can approve this as long as we're aligned that the function's signature will change to match DataFrame.rename (or get as close as possible) in future PRs. Alternatively, if you'd like to tackle #662 in this PR, or even go all the way to get the signatures to match in this PR, I can review that too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not following. this test was named test_rename_exception. since i added a new exception to the rename function, I added a clause to test the new exception.

what would you like to see changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants